lib: prefer `call()` over `apply()` if argument list is not array #60796

LiviaMedeiros · 2025-11-21T08:36:24Z

All three of FunctionPrototypeCall, FunctionPrototypeApply, and ReflectApply are used across codebase without clear guidelines on when each should be preferred.
The inconsistency gets to the point where identical lines within same file may use different approach:

node/lib/internal/fs/utils.js

Lines 494 to 498 in b8b4350

    
           function BigIntStats(dev, mode, nlink, uid, gid, rdev, blksize, 
        
                                ino, size, blocks, 
        
                                atimeNs, mtimeNs, ctimeNs, birthtimeNs) { 
        
             ReflectApply(StatsBase, this, [dev, mode, nlink, uid, gid, rdev, blksize, 
        
                                            ino, size, blocks]);

node/lib/internal/fs/utils.js

Lines 522 to 526 in b8b4350

    
           function Stats(dev, mode, nlink, uid, gid, rdev, blksize, 
        
                          ino, size, blocks, 
        
                          atimeMs, mtimeMs, ctimeMs, birthtimeMs) { 
        
             FunctionPrototypeCall(StatsBase, this, dev, mode, nlink, uid, gid, rdev, 
        
                                   blksize, ino, size, blocks);

This PR brings a bit more consistency by using FunctionPrototypeCall whenever the argument list is not an already predefined array.

In terms of performance, currently apply with short predefined array of arguments is almost as fast as call with predefined variables (i.e. not ...args nor args[0], args[1], ...), while call is 2-3 times faster than apply that allocates new array for each call.

This probably can become a linter rule.

nodejs-github-bot · 2025-11-21T08:36:29Z

Review requested:

@nodejs/crypto
@nodejs/loaders
@nodejs/net

codecov · 2025-11-21T09:31:07Z

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.55%. Comparing base (93fc880) to head (0eee566).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/tls/wrap.js	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60796      +/-   ##
==========================================
+ Coverage   88.53%   88.55%   +0.01%     
==========================================
  Files         703      703              
  Lines      208260   208252       -8     
  Branches    40156    40166      +10     
==========================================
+ Hits       184393   184421      +28     
+ Misses      15881    15832      -49     
- Partials     7986     7999      +13

Files with missing lines	Coverage Δ
lib/assert.js	`98.23% <100.00%> (ø)`
lib/dgram.js	`97.45% <100.00%> (-0.01%)`	⬇️
lib/fs.js	`98.18% <100.00%> (+<0.01%)`	⬆️
lib/internal/child_process.js	`95.08% <100.00%> (-0.01%)`	⬇️
lib/internal/cluster/child.js	`98.38% <100.00%> (+<0.01%)`	⬆️
lib/internal/cluster/worker.js	`100.00% <100.00%> (ø)`
lib/internal/crypto/cipher.js	`97.94% <100.00%> (ø)`
lib/internal/crypto/hash.js	`98.93% <100.00%> (ø)`
lib/internal/crypto/sig.js	`95.73% <100.00%> (-0.02%)`	⬇️
lib/internal/crypto/webcrypto.js	`96.37% <100.00%> (+<0.01%)`	⬆️
... and 13 more

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Renegade334

As an aside, it'd be interesting to know whether there's any significant performance difference between the bound FunctionPrototypeApply and the native ReflectApply.

nodejs-github-bot · 2025-11-21T13:45:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/70266/

aduh95 · 2025-11-21T14:35:27Z

This probably can become a linter rule.

Using the following:

diff --git a/lib/eslint.config_partial.mjs b/lib/eslint.config_partial.mjs
index b919708a978..457ae91a987 100644
--- a/lib/eslint.config_partial.mjs
+++ b/lib/eslint.config_partial.mjs
@@ -40,2 +40,10 @@ const noRestrictedSyntax = [
   },
+  {
+    selector: "CallExpression:matches([callee.type='Identifier'][callee.name='FunctionPrototypeApply'], [callee.type='MemberExpression'][callee.property.type='Identifier'][callee.property.name='apply'][arguments.length=2])",
+    message: 'Use `ReflectApply` instead of %Function.prototype.apply%',
+  },
+  {
+    selector: "CallExpression[callee.type='Identifier'][callee.name='ReflectApply'][arguments.2.type='ArrayExpression']",
+    message: 'Use `FunctionPrototypeCall` to avoid creating an ad-hoc array',
+  },
 ];

I see there are still 33 reported "problems" in lib/. I guess that can be addressed in a followup

aduh95 · 2025-11-21T14:36:42Z

@Renegade334 added backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch.

Why?

Renegade334 · 2025-11-21T14:40:06Z

Why?

Doesn't land, is all.

aduh95 · 2025-11-21T14:42:58Z

Why?

Doesn't land, is all.

It's probably too soon to assess, the staging branch is many commits behind

LiviaMedeiros · 2025-11-21T15:29:33Z

As an aside, it'd be interesting to know whether there's any significant performance difference between the bound FunctionPrototypeApply and the native ReflectApply.

From a brief comparison on main and v24.11.1, their performance as primordials using --expose-internals looks exactly the same. Follow-up, moving away from FunctionPrototypeApply purely for consistency, probably wouldn't hurt.

Also for the opposite (FunctionPrototypeCall(something, something, ...args)), the only occurrence i've found was:

node/lib/internal/webstreams/util.js

Lines 169 to 178 in b8b4350

    
           // This implements "invoke a callback function type" for callback functions that return a promise. 
        
           // See https://webidl.spec.whatwg.org/#es-invoking-callback-functions 
        
           async function invokePromiseCallback(fn, thisArg, ...args) { 
        
             return FunctionPrototypeCall(fn, thisArg, ...args); 
        
           } 
        
           function createPromiseCallback(name, fn, thisArg) { 
        
             validateFunction(fn, name); 
        
             return (...args) => invokePromiseCallback(fn, thisArg, ...args); 
        
           }

I see there are still 33 reported "problems" in lib/. I guess that can be addressed in a followup

Thanks! At least some of them come from specific files that had prototype primordials removed and restricted:

node/lib/eslint.config_partial.mjs

Lines 513 to 527 in b8b4350

    
             'lib/_http_*.js', 
        
             'lib/_tls_*.js', 
        
             'lib/http.js', 
        
             'lib/http2.js', 
        
             'lib/internal/http.js', 
        
             'lib/internal/http2/*.js', 
        
             'lib/tls.js', 
        
             'lib/zlib.js', 
        
           ], 
        
           rules: { 
        
             'no-restricted-syntax': [ 
        
               ...noRestrictedSyntax, 
        
               { 
        
                 selector: 'VariableDeclarator:has(.init[name="primordials"]) Identifier[name=/Prototype[A-Z]/]:not([name=/^(Object|Reflect)(Get|Set)PrototypeOf$/])', 
        
                 message: 'Do not use prototype primordials in this file.',

AFAICT it would be okay to suppress this rule or use .call() instead of primordial, but it's better to be separate PR with benchmark CI.

lib: prefer call() over apply() if argument list is not array

0eee566

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 21, 2025

jsumners-nr approved these changes Nov 21, 2025

View reviewed changes

Renegade334 approved these changes Nov 21, 2025

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2025

Renegade334 removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Nov 21, 2025

aduh95 removed the backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch. label Nov 21, 2025

aduh95 approved these changes Nov 21, 2025

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 21, 2025

cjihrig approved these changes Nov 21, 2025

View reviewed changes

lpinca approved these changes Nov 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

lib: prefer `call()` over `apply()` if argument list is not array #60796

lib: prefer `call()` over `apply()` if argument list is not array #60796

Uh oh!

LiviaMedeiros commented Nov 21, 2025

Uh oh!

nodejs-github-bot commented Nov 21, 2025

Uh oh!

codecov bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

Renegade334 left a comment

Uh oh!

nodejs-github-bot commented Nov 21, 2025

Uh oh!

aduh95 commented Nov 21, 2025 •

edited

Loading

Uh oh!

aduh95 commented Nov 21, 2025

Uh oh!

Renegade334 commented Nov 21, 2025

Uh oh!

aduh95 commented Nov 21, 2025

Uh oh!

LiviaMedeiros commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	function BigIntStats(dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks,
	atimeNs, mtimeNs, ctimeNs, birthtimeNs) {
	ReflectApply(StatsBase, this, [dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks]);

	function Stats(dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks,
	atimeMs, mtimeMs, ctimeMs, birthtimeMs) {
	FunctionPrototypeCall(StatsBase, this, dev, mode, nlink, uid, gid, rdev,
	blksize, ino, size, blocks);

Uh oh!

lib: prefer call() over apply() if argument list is not array #60796

Are you sure you want to change the base?

lib: prefer call() over apply() if argument list is not array #60796

Uh oh!

Conversation

LiviaMedeiros commented Nov 21, 2025

Uh oh!

nodejs-github-bot commented Nov 21, 2025

Uh oh!

codecov bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Renegade334 left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Nov 21, 2025

Uh oh!

aduh95 commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Nov 21, 2025

Uh oh!

Renegade334 commented Nov 21, 2025

Uh oh!

aduh95 commented Nov 21, 2025

Uh oh!

LiviaMedeiros commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lib: prefer `call()` over `apply()` if argument list is not array #60796

lib: prefer `call()` over `apply()` if argument list is not array #60796

codecov bot commented Nov 21, 2025 •

edited

Loading

aduh95 commented Nov 21, 2025 •

edited

Loading